fix(rspec): quarantine and abort edge cases (retries, dry-run, before(:context), hook order) by dfrankland · Pull Request #1213 · trunk-io/analytics-cli

dfrankland · 2026-09-30T17:19:15Z

Summary

This PR started as a one-line switch from before to prepend_before for the quarantine-abort hook, which a customer suggested. Reviewing the gem for similar logic errors found five more bugs, and this PR fixes all of them. Each one was reproduced against a locally built gem before being fixed.

Hook order. The abort check now uses prepend_before, so it runs ahead of before hooks that were set up before trunk_spec_helper was required. around hooks still wrap a skipped example; a before hook can't prevent that.
Abort with in-place retries turned a real failure into a pass. RSpec (and knapsack_pro) start no new examples once RSpec.world.wants_to_quit is set. So the abort hook only ever fired when rspec-retry re-ran the same example, and its skip replaced the failure. With TRUNK_QUARANTINE_QUERY_FAILURE_EXIT=true and rspec-retry, an example that always fails finished as 1 example, 0 failures, 1 pending with exit 0. The hook now re-raises the first failure that aborted the run, so the retry fails again right away without running the test body. The replayed failure skips the quarantine lookup. Examples that never ran are still skipped.
rspec --dry-run uploaded every example as passed. In a dry run RSpec reports each example as passed without running it. The listener now records and uploads nothing when dry_run? is set.
A quarantined before(:context) failure still failed the build. RSpec fails each example through set_exception, where quarantining applies, but the group itself still returns false, and that sets the exit code. The same quarantined error exited 0 when raised in the test body and 1 when raised in before(:context). When Trunk is active and every example's failure was quarantined, the group now reports as passed. A green status alone isn't enough: a pending example over a crashing before(:context) ends up :passed with the error hidden, and plain RSpec fails that run.
A quarantined example that also failed in an after hook kept only the last error. The recorded failure (trunk_quarantined_exception) was overwritten, so Trunk received cleanup error instead of the real failure. Failures in the same attempt now accumulate in a MultipleExceptionError, as RSpec does for non-quarantined examples. When an example is re-run in place, each attempt starts fresh.
Test durations were truncated to whole seconds. Ruby passed Time#to_i, and Rust built the timestamps with zero nanoseconds. MutTestReport#add_test now takes f64 epoch seconds and keeps microseconds. chrono now requires at least 0.4.35, the first version with DateTime::from_timestamp_micros. Its only callers are the gem and the Rust tests. It is also exported to wasm, where f64 maps to a JS number.

Testing

test/quarantine_spec.rb covers fixes 2–6 and the follow-ups from review, and test/abort_hook_spec.rb covers the hook order (1). test/support/trunk_harness.rb runs sandboxed examples with Trunk active against a fake report, so no API is needed. I checked that each new example fails when its fix is reverted. The suite also passes with TRUNK_LOCAL_UPLOAD_DIR set, as it is in CI.
bundle exec rake test: 34 examples, 0 failures. That includes namespace_spec, so the new ExampleGroup patch adds nothing outside RSpec::Trunk.
cargo test -p test_report passes, including new unit tests for the timestamp conversion. cargo fmt --check is clean, and cargo check -p test_report --features wasm builds.
End to end with the rebuilt gem, using real rspec-retry, --dry-run, and a quarantine answer stubbed on the report:
- rspec-retry with a failed lookup and abort on: exit 1 (was 0)
- quarantined before(:context) failure: exit 0 (was 1)
- crashing before(:context) over a pending example: exit 1, the same as plain RSpec
- dry run: no bundle written (was one bundle with every test passing)
- after hook error: both errors reported

🤖 Generated with Claude Code

With TRUNK_QUARANTINE_QUERY_FAILURE_EXIT=true, examples after a failed quarantine lookup are skipped from a before(:example) hook. A plain `before` is appended, so any before hooks the suite configured before requiring trunk_spec_helper still ran (DB setup, fixtures, etc.) for an example that was then skipped. Register it with prepend_before so the skip happens first. Hook registration moves into RSpec::Trunk.install(config, run) so it can be exercised against a sandboxed configuration. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

trunk-io · 2026-09-30T17:19:18Z

😎 Merged successfully - details.

codecov-commenter · 2026-09-30T17:32:37Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.96%. Comparing base (209c229) to head (ac6f808).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1213      +/-   ##
==========================================
+ Coverage   83.74%   83.96%   +0.22%     
==========================================
  Files          74       74              
  Lines       17745    17749       +4     
==========================================
+ Hits        14860    14903      +43     
+ Misses       2885     2846      -39

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

trunk-staging-io · 2026-09-30T17:34:25Z

Failed Test	Failure Summary	Logs
`pending_quarantine_test should be quarantined when run with variant`	A test marked as pending was expected to fail but unexpectedly passed.	Logs ↗︎
`variant_quarantine_test should be quarantined when run with variant`	A test expected the sum of 2 + 2 to be 5, but it was actually 4, indicating a failing assertion.	Logs ↗︎

_{View Full Report ↗︎ ⋅ Docs}

trunk-io · 2026-09-30T17:46:36Z

Failed Test	Failure Summary	Logs
`pending_quarantine_test should be quarantined when run with variant`	A test marked as pending was expected to fail but unexpectedly passed.	Logs ↗︎
`variant_quarantine_test should be quarantined when run with variant`	A test expected the sum of 2 + 2 to be 5, but it was actually 4, indicating a failing assertion.	Logs ↗︎

_{View Full Report ↗︎ ⋅ Docs}

- Abort + in-place retries: RSpec starts no new examples once wants_to_quit is set, so the abort hook only fired for an example re-run in place by rspec-retry, and its skip replaced the failure: an always-failing example finished pending and the run exited 0. The hook now re-raises the failure that aborted the run. - --dry-run: every example was recorded as passed and uploaded. The listener now records and uploads nothing in a dry run. - before(:context) errors: each example's failure was quarantined, but the group still returned false, failing the run. A group whose examples all passed or are pending now reports as passed. - A quarantined example that failed again in an after hook kept only the last error; failures now accumulate in a MultipleExceptionError. - Start/finish times were truncated to whole seconds. add_test now takes f64 epoch seconds and keeps microseconds. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

dfrankland · 2026-09-30T17:55:55Z

@claude review

dfrankland · 2026-09-30T18:43:37Z

Code review

I found 8 issues. Two can hide real failures and one mis-reports them; the rest are smaller.

Bugs

1. ExampleGroupExtension hides a crashing before(:context) when the group's examples are pending (trunk_spec_helper.rb#L318)
The group counts as passed whenever every example ends up :passed/:pending, even if Trunk quarantined none of them. For a pending example, fail_with_exception → display_exception= puts the error in pending_exception and leaves @exception nil. mark_pending! never runs, so finish records :passed (rspec-core 3.13.6, example.rb L396–404 and L478–495). Plain RSpec still returns false from the group and exits 1. With the gem loaded, before(:context) { raise 'db down' } over pending examples exits 0 and the error is never shown. Suggest requiring that every non-green example was actually quarantined (e.g. metadata[:trunk_quarantined_exception] is set), not only that its status is green.

2. abort_run keeps only the last failure, so a re-run can replay the after-hook error instead of the real one (trunk_spec_helper.rb#L246)
With TRUNK_QUARANTINE_QUERY_FAILURE_EXIT=true and the lookup down, the body raises real bug and abort_run stores it. An after hook then raises cleanup, the lookup fails again, and @abort_failures[example] is overwritten. When rspec-retry re-runs the example, the prepended hook raises only cleanup, so real bug is lost from the output and the upload. record_quarantined already combines failures into a MultipleExceptionError; abort_run should do the same, or keep the first failure.

3. record_quarantined carries failures across in-place re-runs (trunk_spec_helper.rb#L253)
metadata[:trunk_quarantined_exception] is never cleared between attempts. An example re-run without :retry_attempts (an around hook calling ex.run twice, or another retry tool) uploads attempt 1 as MultipleExceptionError(A, B). Before this change it was just B. rspec-retry isn't affected, because later attempts aren't quarantine candidates.

Smaller issues

4. The harness comment is wrong for ExampleGroupExtension (trunk_harness.rb#L9)
It says the patches "do nothing while no run is current", but ExampleGroupExtension#run never checks Trunk.current_run. After the first with_trunk, every group in the unit suite uses the changed pass/fail logic, so results can depend on test order. Returning passed unchanged when Trunk.current_run is nil fixes both the test suite and the comment.

5. Re-raising the stored abort failure starts another quarantine lookup (trunk_spec_helper.rb#L89)
The raised failure goes back through ExampleExtension#set_exception. For a re-run without :retry_attempts, it's still a quarantine candidate, so is_quarantined runs again and the "checking… / exiting early" lines print again. If the lookup has recovered and says the test is quarantined, the old failure is quarantined and the example passes, even though its body never ran on that attempt.

6. around hooks still run for skipped examples (trunk_spec_helper.rb#L79-L82)
prepend_before only runs ahead of other before hooks. A suite's config.around (DatabaseCleaner, VCR, …) still does its full setup and teardown around an example that's about to be skipped. That's fine to accept, but the comment says a skipped example won't "pay for (or be affected by) the suite's setup", and that isn't quite true.

7. timestamp_from_epoch_secs builds the Timestamp by hand (report.rs#L952-L959)
prost_wkt_types already has impl From<DateTime<Utc>> for Timestamp, and the CLI uses it (chrono::Utc::now().into()). The body could be DateTime::from_timestamp_micros((secs * 1_000_000.0).round() as i64).unwrap_or_default().into().

8. chrono lower bound is too low for DateTime::from_timestamp_micros (report.rs#L954)
test_report/Cargo.toml asks for chrono = "0.4.33", but DateTime::from_timestamp_micros first appears in 0.4.35 (it's absent from src/datetime/mod.rs at v0.4.33 and v0.4.34). Only the lockfile's 0.4.42 keeps this building. Raising the bound to 0.4.35 fixes it.

Checked against rspec-core 3.13.6 and the chrono tags; I didn't run the reproductions.

🤖 Generated with Claude Code

- A group RSpec failed now passes only when Trunk is active and every example's failure was quarantined. A green status alone let a crashing before(:context) over pending examples (error hidden in pending_exception, status :passed) exit 0 where plain RSpec exits 1. - The abort replays the first failure (the one whose lookup failed), not whichever came last, and the replay skips the quarantine lookup. - Quarantined failures combine within an attempt only; the prepended before hook resets them as each attempt starts. - Comment on the prepended hook no longer claims around hooks are skipped. - timestamp_from_epoch_secs uses prost_wkt_types' From<DateTime<Utc>>. - chrono >= 0.4.35, where DateTime::from_timestamp_micros first appears. - Test harness: FakeReport handles try_save, which CI reaches because it sets TRUNK_LOCAL_UPLOAD_DIR; it also counts lookups. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

dfrankland changed the title ~~fix(rspec): run the quarantine-abort check ahead of other before hooks~~ fix(rspec): quarantine and abort edge cases (retries, dry-run, before(:context), hook order) Sep 30, 2026

dfrankland and others added 2 commits September 30, 2026 18:53

chore(rspec): trim comments to the non-obvious whys

ac6f808

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

dfrankland requested review from TylerJang27, acatxnamedvirtue and max-trunk September 30, 2026 21:39

acatxnamedvirtue approved these changes Oct 5, 2026

View reviewed changes

max-trunk approved these changes Oct 6, 2026

View reviewed changes

trunk-io Bot merged commit 23474de into main Oct 6, 2026
35 checks passed

Navigation Menu

Repository navigation

fix(rspec): quarantine and abort edge cases (retries, dry-run, before(:context), hook order) - #1213

fix(rspec): quarantine and abort edge cases (retries, dry-run, before(:context), hook order)#1213
trunk-io[bot] merged 4 commits into
mainfrom
rspec-prepend-abort-hook

dfrankland commented Sep 30, 2026 •

edited

Loading

Uh oh!

trunk-io Bot commented Sep 30, 2026 •

edited

Loading

Uh oh!

codecov-commenter commented Sep 30, 2026 •

edited

Loading

Uh oh!

trunk-staging-io Bot commented Sep 30, 2026 •

edited

Loading

Uh oh!

trunk-io Bot commented Sep 30, 2026 •

edited

Loading

Uh oh!

dfrankland commented Sep 30, 2026

Uh oh!

dfrankland commented Sep 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Conversation

dfrankland commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

trunk-io Bot commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

trunk-staging-io Bot commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trunk-io Bot commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dfrankland commented Sep 30, 2026

Uh oh!

dfrankland commented Sep 30, 2026

Code review

Bugs

Smaller issues

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

dfrankland commented Sep 30, 2026 •

edited

Loading

trunk-io Bot commented Sep 30, 2026 •

edited

Loading

codecov-commenter commented Sep 30, 2026 •

edited

Loading

trunk-staging-io Bot commented Sep 30, 2026 •

edited

Loading

trunk-io Bot commented Sep 30, 2026 •

edited

Loading